[custom channels]: Assert fix for duplicate script key issues in MPP payments #908
Conversation
itest/litd_custom_channels_test.go
Outdated
| // active. | ||
| numHtlcs := 4 | ||
| if mpp { | ||
| numHtlcs += 2 |
There was a problem hiding this comment.
nit: could derive the numHtlcs from invoice amt + default max shard size
There was a problem hiding this comment.
Right, makes sense. That way we can also more easily increase the number of HTLCs later on.
itest/litd_custom_channels_test.go
Outdated
| bobHodlInvoices []assetHodlInvoice | ||
| aliceHodlInvoices []assetHodlInvoice | ||
| assetInvoiceAmt = 100 | ||
| assetInvoiceAmt = 5_000 |
There was a problem hiding this comment.
nit: Bit confused on the math for this change causing MPP to get used.
AFAICT, setting mpp = true below limits the shard size to 80e6 msats / 80000 sats.
But the minted asset seems to be 1,000,000 itest-asset-cents. And it looks like assetInvoiceAmt is denominated in cents also. Not sure if decimal display is being set somewhere I can't find.
I also can't find the exchange rate being used here that would probably help clarify this math.
There was a problem hiding this comment.
Update: I found the rate, from grepping after inspecting the logs, as the config value of mockoracleassetsperbtc. Would still be good to call out.
Also this log msg should be got quote for %d msats, not sats:
lightning-terminal/itest/assets_test.go
Line 1129 in a72e388
So, the quoted sat amount is ~85901 sats, which is over the MPP shard threshold of 80000 sats / 4656 asset units.
There was a problem hiding this comment.
Ah, yes, that's not super apparent. Added a comment to describe that, which then also makes the use of the mpp variable a bit more apparent.
There was a problem hiding this comment.
I didn't grok this either. Thanks for explaining it @jharveyb.
go.mod
Outdated
| github.com/lightninglabs/pool/auctioneerrpc v1.1.2 | ||
| github.com/lightninglabs/pool/poolrpc v1.0.0 | ||
| github.com/lightninglabs/taproot-assets v0.4.2-0.20241125202051-783fb1e5ab03 | ||
| github.com/lightninglabs/taproot-assets v0.4.2-0.20241127150634-c96f36961231 |
There was a problem hiding this comment.
Could bump it now to RC5, right?
There was a problem hiding this comment.
Yes, will do that once the tag is pushed in tapd.
guggero
force-pushed
the
fix-htlc-mpp-duplicate-script-key
branch
from
894800d to
81f25b7
Compare
| aliceSweepTransfer := locateAssetTransfers( | ||
| t.t, aliceTap, sweepTx.TxHash(), | ||
| ) | ||
| t.Logf("Alice's first-level sweep transfer: %v", |
There was a problem hiding this comment.
nit:
This if-else statement could be extracted into its own function, with a string parameter for the Logf statement. This prevents the code duplication that is going on in this test.
Updates the force close HTLC sweep integration test to also do MPP payments, to make sure duplicate script keys due to identical MPP shard HTLCs don't cause an issue.